More cosmetic fixes and clean up - #1025
Conversation
There was a problem hiding this comment.
This one is trickier here, because depending on the CONFIG_TASK_HAVE_PRIORITY_XXX you can end up with memory leaks.
But looking at the code it looks that it doesn't really matter if those allocation failed.
There was a problem hiding this comment.
I think it was already mentioned somewhere, that it looks like KW problem, because fail to alloc from system heap always ends in panic.
There was a problem hiding this comment.
rmalloc_sys() panics if there is no free memory to allocate which practically makes the if(null) a dead code in every place that follows an allocation from RZONE_SYS, and what is more, it suggests the reader that the allocation may really fail at run-time being handled by the callers. If not sure why KW is unable to analyze that path and follow switch (zone & RZONE_TYPE_MASK) correctly.
I am also not sure why the _malloc() just traces error in the switch() default branch and returns NULL ptr since it seems to be a serious logic error, not a run-time problem to be handled in some way. This branch seems to confuse KW. I'd try another approach: panic in the default branch in _malloc() and mark this infinite loop as 'by design' piece to neutralize KW.
|
@lyakh looks like CI builds have failed. |
|
@lyakh Could you plase rebase with master, KW should be OK now wrt sys heap allocations. |
Add a missing preamble in a recently created idc.c Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Interrupt registration can fail, check its return code in idc.c and propagate error to the caller. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The SMP version of init.c calls panic(), therefore it needs panic.h, OTOH the UP version doesn't call panic(), so the header isn't needed there. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
When returning an error, it's usually better to propagate the error code, that caused the termination, than overriding it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
I rebased and extended this series, also moved here cosmetic patches from my interrupt extension PR #1053 |
All trace_event() style macros resolve to _log_message() or __log_message() eventually. This message makes those macros usable as a function, e.g. within "if () else" clauses with or without curly braces. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Using macro arguments multiple times within the macro definition body can lead to repeated expressioon evaluation. To avoid that replace MIN() and MAX() macros with safer versions. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Function-type macros are usually called as "M(...);" with a semicolon in the end. Therefore defining such macros with a semicolon isn't a good practice. This breaks constructs like if (...) M(...); else ...; Fix the dma_set_drvdata() macro to avoid such problems. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Some editors (e.g. emacs) do not take into account preprocessor
conditionals when performing syntax highlighting. I.e. patterns like
if (a) {
if (b) {
do_things();
}
cause those editors to miscalculate the number of opening and closing
braces and thus break code highlighting. Move the opening brace to
after the preprocessor conditional to avoid that.
Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Rearrange a conditional to reduce the number of branches and eliminate a goto. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
As long as the low bits of two numbers are 0, there's no need to right shift those numbers to compare them. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Use a "for" loop instead of simulating it, using a "while" loop. Also avoid needlessly setting a variable inside that loop, where it can just be set once after the loop termination. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
"Formate" and "format" are two different words, in this case it's "format" that is meant. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
| j = 0; | ||
| /* Loop until NULL */ | ||
| while (fir_list[j]) { | ||
| for (j = 0; fir_list[j]; j++) { |
There was a problem hiding this comment.
I suppose this works but what is the improvement?
There was a problem hiding this comment.
I find
for (i = 0; i < N; i++)
better than
i = 0;
while (i < N) {
...
i++;
}
| trace_dmic("dmic_set_config(), cfg mfir_a = %u, mfir_b = %u", | ||
| cfg.mfir_a, cfg.mfir_b); | ||
| trace_dmic("dmic_set_config(), cfg cic_shift = %u", cfg.cic_shift) | ||
| trace_dmic("dmic_set_config(), cfg cic_shift = %u", cfg.cic_shift); |
There was a problem hiding this comment.
No wonder indenting in editor has behaved strange. Thanks!
|
|
||
| #define MIN(a, b) (((a) < (b)) ? (a) : (b)) | ||
| #define MAX(a, b) (((a) > (b)) ? (a) : (b)) | ||
| #define MIN(a, b) ({ \ |
There was a problem hiding this comment.
I'm curious, why is this better?
There was a problem hiding this comment.
if you have MIN(x++, y), then with the first version you'd get
(x++ > y) ? x++ : y;
i.e. you'd have x incremented twice. With the second version you'd correctly have it incremented once. Same for any MIN(f(x), y) - f(x) would be called twice with the first version.
6 participants
A couple of cosmetic fixes and improvements. They don't have anything to do with KW any more, since the only patch, that was dealing with a KW complaint, was fixing a false positive - thanks to all for pointing out! So, all what's left is just clean up and cosmetic stuff.